fix(Webhook): unknown error in catch block - #73
Conversation
|
✔️ Deploy Preview for topgg-node-sdk-docs ready! 🔨 Explore the source changes: 79bb5a7 🔍 Inspect the deploy log: https://app.netlify.com/sites/topgg-node-sdk-docs/deploys/619b895c6689420008f41f14 😎 Browse the preview: https://deploy-preview-73--topgg-node-sdk-docs.netlify.app |
Xetera
left a comment
Xetera
left a comment
There was a problem hiding this comment.
Sorry for the long wait for the review 😞
| res.sendStatus(204); | ||
| } | ||
| } catch (err) { | ||
| this.options.error?.(err); |
There was a problem hiding this comment.
I'm confused, what exactly is getting thrown from functions that isn't an error? If we're handling all errors with a 500 response, shouldn't they also be treated as an error on the library side? Maybe the correct move here should be to change the type of the error callback from Error to unknown
There was a problem hiding this comment.
Are we sure we want to do this? It would be a breaking change. I'm not opposed but a major bump seems unnecessary.
There was a problem hiding this comment.
Well, this is also a breaking change technically, just more subtle
Xetera
left a comment
Xetera
left a comment
There was a problem hiding this comment.
Alright let's just get this in and remember to do the type change for next major release
2 participants
No description provided.